Repository navigation
reject the request promise when reading a response body fails - #376
Merged
Merged
Conversation
…issue apache#375 When the connection breaks off after the response headers, reading the body fails. For JSON responses that error was swallowed and the request resolved with `''`; for text and binary responses it escaped the un-awaited responseHandler, leaving the request promise unsettled and causing an unhandled rejection. Read JSON bodies as text before parsing them, so that only parse errors are ignored, and chain responseHandler in relax() so that any failure rejects the request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
If the connection breaks off after the response headers, reading the body fails. nano 11 currently swallows that failure. JSON responses resolve with
''. For text, multipart and binary responses, the error escapes the un-awaitedresponseHandler, so the request promise never settles and Node reports an unhandled rejection. See #375 for details and a reproduction.This PR makes two changes to
lib/nano.js:response.text(), thenJSON.parse(). Only the parse step is inside the try/catch. A complete body that isn't valid JSON still resolves''as before, and a failed read now propagates. This keeps the "ignore unparseable bodies" behaviour without inspecting error types (SyntaxErrorvsTypeError).relax(): the.then()callback now returns theresponseHandler(…)promise, so the existing.catch()handles any failure from it. A body-read failure then rejects the same way a connection failure before the headers already does:error happened in your connection. Reason: SocketError: other side closed. The promise can't settle twice:responseHandlercallsresolveas its last step, after everyawait. The error path (responseHandler(e, …)with a non-Response) only callsreject.Unchanged, on purpose:
''and the request still rejects with the HTTP status code. The caller already gets a rejection with the most useful information, so I left that branch alone.opts.stream): these go throughstreamResponseHandlerand pipe the body into the returned stream. That path doesn't have this problem.Testing recommendations
npm testpasses: 238 tests, including 4 new ones intest/nano.responsebody.test.js:'')text/plainbody breaks off → rejects (previously never settled; unhandled rejection)application/octet-streambody breaks off → rejects (same as above)application/jsonbody that isn't valid JSON → still resolves''Without the change to
lib/nano.js, the first three fail and the fourth passes.The new tests use a real
http.createServeron127.0.0.1(random port) instead of undici'sMockAgent.MockAgentalways delivers the full reply body and then completes, so it can't simulate a connection that drops partway through the body. The server promisescontent-length: 100, writes part of the body, then destroys the socket. undici then raises the sameTypeError: terminated(causeSocketError: other side closed) as it does against a real CouchDB. The file doesn't loadtest/mock.js, andnode --testruns each file in its own process, so the other tests' globalMockAgent(which hasdisableNetConnect()) doesn't apply here.Also checked:
enginesminimum) and Node 24.21.0.standardreports no errors in the new test file and no new errors inlib/nano.js.node --test --experimental-test-coveragecovers all changed lines.GitHub issue number
Fixes #375
Related Pull Requests
None.
Checklist
This PR was prepared with the help of Claude Code (an AI coding assistant) and reviewed by me before submission. The commit carries a
Co-Authored-Bytrailer, as recommended by the ASF generative tooling guidance.🤖 Generated with Claude Code